Implement update for `remove-snapshots` action by grihabor · Pull Request #1561 · apache/iceberg-python

grihabor · 2025-01-22T16:23:29Z

No description provided.

kevinjqliu

Thanks! This was one of the missing update functions mentioned in #952

Do you mind also including some tests? Similar to

Lines 663 to 682 in 2cd4e78

    
           def test_apply_remove_properties_update(table_v2: Table) -> None: 
        
               base_metadata = update_table_metadata( 
        
                   table_v2.metadata, 
        
                   (SetPropertiesUpdate(updates={"test_a": "test_a", "test_b": "test_b", "test_c": "test_c", "test_d": "test_d"}),), 
        
               ) 
        
               new_metadata_no_removal = update_table_metadata(base_metadata, (RemovePropertiesUpdate(removals=[]),)) 
        
               assert base_metadata == new_metadata_no_removal 
        
               new_metadata = update_table_metadata(base_metadata, (RemovePropertiesUpdate(removals=["test_a", "test_c"]),)) 
        
               assert base_metadata.properties == { 
        
                   "read.split.target.size": "134217728", 
        
                   "test_a": "test_a", 
        
                   "test_b": "test_b", 
        
                   "test_c": "test_c", 
        
                   "test_d": "test_d", 
        
               } 
        
               assert new_metadata.properties == {"read.split.target.size": "134217728", "test_b": "test_b", "test_d": "test_d"}

grihabor · 2025-01-22T16:31:25Z

Sure! Thanks for the fast review

kevinjqliu

left some comments, looks like theres also a linter error, do you mind running make lint locally?

tests/table/test_init.py

kevinjqliu · 2025-01-22T21:17:35Z

pyiceberg/table/update/__init__.py



+@_apply_table_update.register(RemoveSnapshotsUpdate)
+def _(update: RemoveSnapshotsUpdate, base_metadata: TableMetadata, context: _TableMetadataUpdateContext) -> TableMetadata:


this is the java reference

https://github.com/apache/iceberg/blob/be6e9daf8901cdee63197e77fcf95624bb694f39/core/src/main/java/org/apache/iceberg/TableMetadata.java#L1396-L1426

kevinjqliu · 2025-01-22T21:18:09Z

pyiceberg/table/update/__init__.py

+def _(update: RemoveSnapshotsUpdate, base_metadata: TableMetadata, context: _TableMetadataUpdateContext) -> TableMetadata:
+    for remove_snapshot_id in update.snapshot_ids:
+        if remove_snapshot_id == base_metadata.current_snapshot_id:
+            raise ValueError(f"Can't remove current snapshot id {remove_snapshot_id}")


should we block the current snapshot?

I'm not an expert in iceberg spec, but it's not clear what should happen if you try to remove the current snapshot.

I'm also not sure if I should update parent_snapshot_id in every snapshot that was referencing removed snapshots

Decided to set parent_snapshot_id to None if the parent is gone

it's not clear what should happen if you try to remove the current snapshot.

im looking at the java implementation for answers, i think you can just remove the current snapshot... because you can have an empty table with no snapshots

Created a separate pr for remove-snapshot-ref and added a unit test there #1598

grihabor · 2025-01-23T21:50:03Z

Hey @kevinjqliu, ready for another review round. I had to cherry pick the changes from #822 to reuse the code that removes refs

grihabor · 2025-01-27T13:19:05Z

Fixed the linters

grihabor · 2025-01-30T12:42:14Z

Removed corresponding statistics files and entries from snapshot log

kevinjqliu

I added a few comments to the PR. I think we might need some integrations tests to make sure the behavior aligns with the java implementation.

Also, looks like remove_tag and remove_branch are unrelated to this PR, perhaps we can move them to a separate PR.

kevinjqliu · 2025-01-30T01:45:40Z

pyiceberg/table/update/__init__.py

+def _(update: RemoveSnapshotsUpdate, base_metadata: TableMetadata, context: _TableMetadataUpdateContext) -> TableMetadata:
+    for remove_snapshot_id in update.snapshot_ids:
+        if remove_snapshot_id == base_metadata.current_snapshot_id:
+            raise ValueError(f"Can't remove current snapshot id {remove_snapshot_id}")


it's not clear what should happen if you try to remove the current snapshot.

im looking at the java implementation for answers, i think you can just remove the current snapshot... because you can have an empty table with no snapshots

kevinjqliu · 2025-02-01T00:39:52Z

tests/table/test_init.py

+    assert len(table_v2.metadata.snapshots) == 2
+    assert len(table_v2.metadata.snapshot_log) == 2
+    assert len(table_v2.metadata.refs) == 2
+    update = RemoveSnapshotsUpdate(snapshot_ids=[3051729675574597004])


nit can you make 3051729675574597004 a constant for readability?

Added constants REMOVE_SNAPSHOT and KEEP_SNAPSHOT

kevinjqliu · 2025-02-01T00:41:35Z

tests/table/test_init.py

+    with pytest.raises(ValueError, match="Can't remove current snapshot id 3055729675574597004"):
+        update_table_metadata(table_v2.metadata, (update,))
+
+


lets also add some tests for RemoveSnapshotRefUpdate

Created a separate pr for remove-snapshot-ref and added a unit test there #1598

pyiceberg/table/update/__init__.py

grihabor

Thank you for review! Could you explain which kind of integration tests you want? Like pyspark integration with expire_snapshots call?

pyiceberg/table/update/__init__.py

grihabor · 2025-02-01T09:29:37Z

pyiceberg/table/update/__init__.py

+def _(update: RemoveSnapshotsUpdate, base_metadata: TableMetadata, context: _TableMetadataUpdateContext) -> TableMetadata:
+    for remove_snapshot_id in update.snapshot_ids:
+        if remove_snapshot_id == base_metadata.current_snapshot_id:
+            raise ValueError(f"Can't remove current snapshot id {remove_snapshot_id}")


Created a separate pr for remove-snapshot-ref and added a unit test there #1598

grihabor · 2025-02-01T09:30:06Z

tests/table/test_init.py

+    with pytest.raises(ValueError, match="Can't remove current snapshot id 3055729675574597004"):
+        update_table_metadata(table_v2.metadata, (update,))
+
+


Created a separate pr for remove-snapshot-ref and added a unit test there #1598

grihabor · 2025-02-01T09:34:39Z

tests/table/test_init.py

+    assert len(table_v2.metadata.snapshots) == 2
+    assert len(table_v2.metadata.snapshot_log) == 2
+    assert len(table_v2.metadata.refs) == 2
+    update = RemoveSnapshotsUpdate(snapshot_ids=[3051729675574597004])


Added constants REMOVE_SNAPSHOT and KEEP_SNAPSHOT

Fokko

Left some small comments, but this looks great @grihabor

Fokko · 2025-02-03T11:14:50Z

pyiceberg/table/update/__init__.py

+
+    snapshots = [
+        (s.model_copy(update={"parent_snapshot_id": None}) if s.parent_snapshot_id in update.snapshot_ids else s)
+        for s in base_metadata.snapshots


Can we make this a little more verbose:

Suggested change

for s in base_metadata.snapshots

for snapshot in base_metadata.snapshots

Sure fb8f350

pyiceberg/table/update/__init__.py

Part of #1561 Closes #822 --------- Co-authored-by: Fokko Driesprong <fokko@apache.org>

kevinjqliu · 2025-02-16T20:41:37Z

now that #1598 is merged, @grihabor could you rebase main?

…date

grihabor · 2025-02-17T11:56:36Z

@kevinjqliu Sure, merged wth upstream main

kevinjqliu

LGTM!

Implement update for remove-snapshot action

b6ba319

grihabor changed the title ~~Implement update for remove-snapshot action~~ Implement update for remove-snapshots action Jan 22, 2025

kevinjqliu reviewed Jan 22, 2025

View reviewed changes

grihabor added 2 commits January 22, 2025 20:19

Add tests

88064c0

Make sure parent_snapshot_id is consistent

818f771

kevinjqliu reviewed Jan 22, 2025

View reviewed changes

grihabor added 3 commits January 23, 2025 15:38

Set parent_snapshot_id to None

9b31690

cherry-pick: Support Remove Branch or Tag APIs (apache#822)

c5cbfe4

Make sure refs are removed when removing snapshots

29feaf7

Fix ruff and pydocstyle

9e3f7c7

grihabor added 3 commits January 30, 2025 12:52

Remove statistics

b9c912d

Fix name collision

d515e74

Remove entries from snapshot log

c7481fe

kevinjqliu reviewed Feb 1, 2025

View reviewed changes

grihabor mentioned this pull request Feb 1, 2025

Implement update for remove-snapshot-ref action #1598

Merged

grihabor added 2 commits February 1, 2025 10:34

Introduce constants for test readability

da4db9f

Allow to remove main branch

19e17f0

grihabor force-pushed the remove-snapshots-update branch from 28c6657 to 19e17f0 Compare February 1, 2025 09:45

Revert changes (moved to apache#1598)

32e1e85

grihabor force-pushed the remove-snapshots-update branch from c8c63ea to 32e1e85 Compare February 1, 2025 09:53

grihabor commented Feb 1, 2025

View reviewed changes

Fokko reviewed Feb 3, 2025

View reviewed changes

Fokko added a commit that referenced this pull request Feb 14, 2025

Implement update for remove-snapshot-ref action (#1598)

dd58ac1

Part of #1561 Closes #822 --------- Co-authored-by: Fokko Driesprong <fokko@apache.org>

grihabor added 2 commits February 17, 2025 12:53

Merge remote-tracking branch 'upstream/main' into remove-snapshots-up…

e68dbb9

…date

Use verbose variable names

fb8f350

Fokko approved these changes Feb 17, 2025

View reviewed changes

kevinjqliu approved these changes Feb 17, 2025

View reviewed changes

kevinjqliu merged commit 19148d3 into apache:main Feb 17, 2025
7 checks passed

grihabor mentioned this pull request Feb 19, 2025

Add test to ensure every table update has corresponding _apply_table_update function #952

Open

	def test_apply_remove_properties_update(table_v2: Table) -> None:
	base_metadata = update_table_metadata(
	table_v2.metadata,
	(SetPropertiesUpdate(updates={"test_a": "test_a", "test_b": "test_b", "test_c": "test_c", "test_d": "test_d"}),),
	)

	new_metadata_no_removal = update_table_metadata(base_metadata, (RemovePropertiesUpdate(removals=[]),))

	assert base_metadata == new_metadata_no_removal

	new_metadata = update_table_metadata(base_metadata, (RemovePropertiesUpdate(removals=["test_a", "test_c"]),))

	assert base_metadata.properties == {
	"read.split.target.size": "134217728",
	"test_a": "test_a",
	"test_b": "test_b",
	"test_c": "test_c",
	"test_d": "test_d",
	}
	assert new_metadata.properties == {"read.split.target.size": "134217728", "test_b": "test_b", "test_d": "test_d"}



		@_apply_table_update.register(RemoveSnapshotsUpdate)
		def _(update: RemoveSnapshotsUpdate, base_metadata: TableMetadata, context: _TableMetadataUpdateContext) -> TableMetadata:

		with pytest.raises(ValueError, match="Can't remove current snapshot id 3055729675574597004"):
		update_table_metadata(table_v2.metadata, (update,))

	for s in base_metadata.snapshots
	for snapshot in base_metadata.snapshots

Conversation

grihabor commented Jan 22, 2025

Uh oh!

kevinjqliu left a comment

Choose a reason for hiding this comment

Uh oh!

grihabor commented Jan 22, 2025

Uh oh!

kevinjqliu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grihabor commented Jan 23, 2025

Uh oh!

grihabor commented Jan 27, 2025

Uh oh!

grihabor commented Jan 30, 2025

Uh oh!

kevinjqliu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

grihabor left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fokko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kevinjqliu commented Feb 16, 2025

Uh oh!

grihabor commented Feb 17, 2025

Uh oh!

kevinjqliu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants